Make 'transaction with too many inputs should be rejected' deterministic - #2455
Make 'transaction with too many inputs should be rejected' deterministic#2455Ergologica wants to merge 1 commit into
Conversation
The test calibrated a wall-clock budget by timing 250K Blake2b256 hashes and then asserted that validation fit in it. The calibration window and the two measurement windows are separate slices of wall time, so on a contended CI runner they get different shares of the CPU and either assertion can flip without anything being wrong with the node. What protects the node here is the block cost limit, not the clock, so assert on cost instead: the transaction is rejected with bsBlockTransactionsCost under the default parameters, its cost with a high enough limit is more than twice the block limit, and any limit below that cost rejects it. The input count is fixed by the generator, so the cost is stable. Also drops the 5M-hash warm-up, taking several seconds off the suite. Closes ergoplatform#2095
|
On the red CI — it is not this PR.
The same CI round on #2456 — which touches forAll(boxesHolderGen) { bh =>
val us = createUtxoState(bh, parameters)
...
val block = validFullBlock(parentOpt = None, us, bh)So what fails is the shared block generator, which neither PR touches — this one changes a single test file,
Not new ground, incidentally — Which is the same shape as #2095, the issue this PR closes: a property test whose generator can produce inputs the property does not hold for. Happy to open a follow-up that makes the generator satisfy the invariant by construction rather than by margin — say the word. |
|
One more thing a reviewer should not have to discover on their own: #2267 already exists for #2095, opened in December, and I did not see it before opening this. They may not actually be rivals. #2095 names the suite generically ("Sometimes ErgoTransactionSpec fails"), and that suite has since been split in two:
#2267 touches the first, rewriting two assertions in the context extension with neg / neg and pos ids properties from This PR touches the second, and one property in it: transaction with too many inputs should be rejected, which warmed up with 5M hashes, calibrated against 250K more, and then asserted on wall-clock time. That is the part that fails on a loaded CI runner, and it is replaced here with an assertion on the measured cost against the block limit (1,000,000 vs 4,362,200 — a 4x margin, no timing). So as far as I can tell the two changes are complementary rather than competing, and both could land. I have no view on how the bounty should be split, and I would rather say this up front than have it surface later — @glasgowm148 for visibility, since the reservation is filed as ErgoDevs/Ergo-Bounties#54. |
|
Correction to my comment above: the cause I named for the I said the bare What I did find while trying, which is reproducible and measurable:
My 500-iteration loop did not finish — it died at roughly 450 with I want to be careful not to make the same mistake twice, so: this is a real leak that I have measured, and a plausible mechanism for a failure that lands in a different suite each run — but I have not traced it to the Happy to open a separate PR for the descriptor leak — it is self-contained and easy to verify with the numbers above — if that is wanted. It is independent of this one. |
|
Opened the descriptor-leak fix as #2460 rather than leaving it as an offer. It is independent of this PR and does not touch anything here. Peak open descriptors for |
1 participant
Closes #2095.
The flaky test
The unstable test is
property("transaction with too many inputs should be rejected"), today inErgoNodeTransactionSpec(it was inErgoTransactionSpecwhen the issue was filed).It calibrated a wall-clock budget on the machine running it:
and then asserted
time0 <= Timeoutfor the cost-limited validation andtime > Timeoutfor the unlimited one.The calibration window and the two measurement windows are three separate slices of wall time. On a contended CI runner they get different shares of the CPU, so either assertion can flip without anything being wrong with the node:
time0 > Timeout-> the first assertion fails;time <= Timeout-> the second assertion fails.A GC pause landing in one window and not the others does the same thing.
The fix
What actually protects the node from this transaction is the block cost limit, not the clock; the cost model exists precisely so that time does not have to be measured. So the assertions are now on cost, which is deterministic:
bsBlockTransactionsCost— unchanged, this assertion was already there and was never the flaky one;1_000_000, full cost4_362_200, a ratio of 4.4. The generator is called asvalidErgoTransactionGenTemplate(0, 0, 2000, trueLeafGen), andboxesGenTemplateis givenminInputs = maxInputs = 2000, so the input count — and hence the cost — is fixed rather than sampled;The 5,000,000-hash warm-up and the 250,000-hash calibration are gone, which also takes several seconds off the suite.
BenchmarkUtilandBlake2b256were used only here and their imports are removed.What is no longer asserted
The old test also implied "the rejection is fast because validation aborts as soon as the limit is passed, instead of validating all 2000 inputs". I did not try to replace that with a timing ratio between the two measurements: it would be less flaky than the absolute budget but still timing-based, and a single GC pause could still invert it. The cost assertions cover the property the node depends on — the transaction costs far more than a block may spend, and any limit below its cost rejects it.
Happy to add an instrumented check of the early abort if you would rather keep that guarantee explicitly covered; it needs a hook that does not exist today.
Note on #2267
#2267 targets this issue but I don't think it can fix it: it edits
ergo-core/src/test/.../ErgoTransactionSpec.scala— a file created in 2024, after the issue was filed — and swaps.toEither.left.getfor aFailure/Successmatch on two fully deterministic tests. The timing-based property it does not touch is the unstable one.sbt "testOnly org.ergoplatform.modifiers.mempool.ErgoNodeTransactionSpec"— 19 tests, green.